-
Notifications
You must be signed in to change notification settings - Fork 411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Storages: New serialization/deserialization for DataTypeString #9608
Conversation
9532cb2
to
a59e61a
Compare
/retest |
1c210d9
to
d664d5b
Compare
ee13f63
to
d925107
Compare
b69d8b3
to
5996395
Compare
Signed-off-by: JaySon-Huang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about define the default serialize string format by STORAGE_FORMAT_CURRENT? JinheLin#4
dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp
Outdated
Show resolved
Hide resolved
Define the default string serialize by storage_format
Co-authored-by: JaySon <[email protected]>
Co-authored-by: JaySon <[email protected]>
Co-authored-by: JaySon <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Others LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
/hold |
Co-authored-by: Lloyd-Pottiger <[email protected]>
Co-authored-by: Lloyd-Pottiger <[email protected]>
Co-authored-by: Lloyd-Pottiger <[email protected]>
/unhold |
@JinheLin: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests
If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
/retest |
What problem does this PR solve?
Issue Number: close #9673
Problem Summary:
What is changed and how it works?
This PR add a new storage format for string, which seperating sizes and chars into different streams, so that we can deserialize string objects in batch.
The new format is not enabled in this PR.
We can change the value ofDefaultSerdesFormat
to enable it.STORAGE_FORMAT_CURRENT
to enable it.For backward compatibility, this PR add a new name for new format, the old name is keepped for old format. Check
DataTypeString::getName()
for details.ColumnFileTiny
andDMFile
will store the name of data type in metadata and get the data type by name when restoring. So reading legacy string data is compatible.ERROR 1105 (HY000): other error for mpp stream: Code: 49, e.displayText() = DB::Exception: CHBlockChunkCodecV1 schema mismatch at column 1, expected String, actual StringV1, e.what() = DB::Exception
.Microbenchmark
sizeX
means the string size is about X bytes. To simulating variable length string, it is not strictly equal to X.none
/lz4
/lw
are the compression algorithm.none
means no compression.lz4
means use lz4 compression algorithm for sizes and chars.lw
means use lightweight compreesion algorithm for sizes, but still use lz4 for chars.Check List
Tests
Side effects
Documentation
Release note